Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed cancel_quit on windows not doing anything #441

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

Shelim
Copy link
Contributor

@Shelim Shelim commented May 31, 2024

At current implementation cancel_quit was completely ignored on Windows - the event that could result in preventing quitting the app, was checked after already sending PostQuitMessage(0).

The PR was reasonably tested on Windows in few different configurations. Namely, the cancel_quit may acquire native_display, so the variable was drop just before entering an event, and restored after. This prevents deadlock in this event.

For easier following the flow, canceling inside quit_requested_event() is actually setting d.quit_requested to false at line 295

@Shelim
Copy link
Contributor Author

Shelim commented May 31, 2024

Actually I find a questionable thing inside my implementation: should the event_handler.quit_requested_event() be called even if it is the programmer who asked to terminate app? Cancelling shouldn't be possible in such case (or it would break Macroquad prevent_quit example, and app would never terminate), but maybe someone want to do some de-intialization on quit, even if it was they who initiate the quit in the first place.

What do you think?

@Shelim
Copy link
Contributor Author

Shelim commented Jun 18, 2024

@not-fl3 Any status update on this?

@not-fl3
Copy link
Owner

not-fl3 commented Jun 18, 2024

Ohhh, missed it. I do not have a windows machine to properly test it, tho it looks like it works fine with wine?

Anyway, lets merge it, will see if there going to be any bug reports!

@not-fl3 not-fl3 merged commit c9adf59 into not-fl3:master Jun 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants